Skip to content

Challenge accepted#4

Open
fcms14 wants to merge 1 commit intoibrahimcesar:mainfrom
fcms14:main
Open

Challenge accepted#4
fcms14 wants to merge 1 commit intoibrahimcesar:mainfrom
fcms14:main

Conversation

@fcms14
Copy link

@fcms14 fcms14 commented Mar 19, 2022

No description provided.

Copy link
Owner

@ibrahimcesar ibrahimcesar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐⭐⭐⭐ and half / 5

Great job!

  • Amazing job in refactoring!
  • Great job fixing the names of tests!
  • You spoted and eliminated all gotchas!
  • The comments above each function have little or no purpose, the functions are almost like one-liners and should explain themselves as they are in the current form
  • Specific to TS: TypeScript has implicit type inference, and could help turn these functions more clean
  • Remove the deprecated! I know is for education purposes, but I would leave from the actual code
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |
 math.ts  |     100 |      100 |     100 |     100 |
----------|---------|----------|---------|---------|-------------------

=============================== Coverage summary ===============================
Statements   : 100% ( 16/16 )
Branches     : 100% ( 0/0 )
Functions    : 100% ( 8/8 )
Lines        : 100% ( 16/16 )
================================================================================

@ibrahimcesar ibrahimcesar self-assigned this Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants